Note about spec vs our writer: your current code may not handle some cases that are ok according to the the spec. An example I know are (multi)polygons - the spec does not mandate any ordering among external / internal rings, while your reader relies on hulls being followed by their holes. AFAIK the old lib will also keep this convention, so it is ok to assume it, but based on the lib and not the spec.

I don't think that it is ok to call this ST_GeodesicLengthWGS84 with Haversine formula as geodesic + WGS84 implies ellipsoid. AFAIK the Esri lib uses Vincenty underneath, which is not included in JTS.

I would
a: rename this function to ST_LengthSphere (redshift has this: https://docs.aws.amazon.com/redshift/latest/dg/ST_LengthSphere-function.html)
b: keep the ESRI geometry library to fill this gap in JTS until a replacement lib is found for ellipsoid computations

Note that the Esri implementation also sums the segment length in more sophisticated way to reduce precision loss when adding doubles. jts also has tools for this: https://github.com/locationtech/jts/blob/master/modules/core/src/main/java/org/locationtech/jts/math/DD.java
Probably this is not that important, but can be another source of small differences.

A bigger issue IMO is that this function is not tested at all. Impala has a few tests generated from the original spatail framwork for Hadoop tests: https://github.com/apache/impala/blob/780e6683a21dae3622744a82f92e155ae06e13f2/testdata/workloads/functional-query/queries/QueryTest/geospatial-esri.test#L397

It would be nice to add multipolygon with holes, as it the most complex case to (de)serialize due to ring orientation rules.

Is there a reason for using equalsTopo? It can hide subtle differences like reordering rings. Not sure if it enforces ring orientation.

IMO equalsExact should be also true here and could catch more bugs. It has a version with tolerance to accept small rounding of coordinates:
https://locationtech.github.io/jts/javadoc/org/locationtech/jts/geom/Geometry.html#equalsExact-org.locationtech.jts.geom.Geometry-double-

I recently created HIVE-29726 about a bug in st_intersects() on multilinestrings. The bug comes from the ESRI lib, so this patch should fix it. Do you think that it makes sense to create a PR for that before this change is merged, or lets fix the issue with that?

Anyway, it would be nice to have a regression test for it, so that the following query returns false:
select st_intersects(st_geomfromtext('LINESTRING(1 0, 1 1)'), st_geomfromtext('MULTILINESTRING((0 0, 0 1), (2 0, 2 1))'));

I won't pull in that change in this, but we can independently have a test, if you already have one as a separate PR after this, to increase the test coverage

ack

24 quadrantSegments is chosen to match Esri lib behavior? A comment could mention this.
(this change is familiar to me from Trino PR description: trinodb/trino#27881)

Do you plan to rename the package? At this point there is not much Esri about these functions anymore. Probably best done in another commit to reduce noise.

I don't plan to do this as part of this PR, considering compat, what are you folks doing? In case you had any packages with esri or so?

Impala's handling of this changed recently, I copied the whole dir from Hive to Impala to be able to make modifications:
apache/impala@d51c3c1

So from Impala's point of view Hive is free to modify / rename / delete these functions.
My plan is to keep the original ESRI lib around for a while to provide compatibility for old workloads, and deprecate it at same point when the new lib seems stable.

I don't have a better solution, but I am worried about interoperability. If there are actually tables with the old format, and users insert to those after this change, then we'll have mixed tables. Reading those will be really painful with any engine other than Hive.

Is there are function that just reads a BINARY and writes it out in the the new format? Would be useful to rewrite tables to get rid of the old format. ST_GeomFromShape does only read old format, right?

In Impala I made the choice of Esri shape vs new format (WKB) a startup flag in https://gerrit.cloudera.org/#/c/24549/ . This is less convenient than accepting both, but I see it as a temporary solution before adding GEOMETRY type and I don't want to commit to a custom format for that time.

What is the plan in Hive about adding GEOMETRY type? That would be an unavoidable breaking change, as functions that return BINARY shapes would need to return geometry (e.g. st_point), which would break queries that relied on the type being BINARY. Adding this new format now would mean that you'll need to be able to read it later, e.g by having ST_GeomFromFromNewHiveShape() (or accepting both formats in ST_GeomFromShape() ).

It is a bit inconsistent that this function expects the shape file format part of a shape, while geometryFromEsriShape expects the header. Could be renamed, e.g. to fromEsriShapeBody.

This function reads only the old format, right? Can you add a comment about that?

Wouldn't it be simpler to keep to ESRI lib for this functionality? https://github.com/Esri/geometry-api-java/blob/master/src/main/java/com/esri/core/geometry/GeoDist.java has its own Vincenty, or the same function could be used an in the original UDF.

Reverted the reimplementation. ST_GeodesicLengthWGS84 delegates to GeometryEngine.geodesicDistanceOnWGS84.

Is normalization really needed? The round trips should not change the order of coordinates. Also, fixing orientation can actually hide bugs.

I still think that equalsExact with tolerance is the right way to check round trips. The order of coordinates can matter, e.g. when representing a route with a linestring reversing it change semantics.

Dropped normalization. Round-trip checks now use equalsExact(expected, actual, EPSILON) directly, so coordinate order and ring orientation must be preserved.

ack

The code uses <=
Which one is correct? Based on googling it seems that ==-1e38 is also considered no data.

<= is correct on read — kept it. Exactly -1e38 is the no-data marker written by ArcView/GDAL and others, and a real measure is never exactly -1e38, so <= catches that marker as well as any value below it. The only actual bug was on the write side: absent M was written as -1e38, which a strict-< reader (e.g. ESRI's own Interop.translateFromAVNaN) would read back as a valid measure. Changed the write value to -Double.MAX_VALUE (matching ESRI's translateToAVNaN), which every reader interprets as no-data.